Skip to content

refactor(no-ticket): pass resolved credentials through credential-helper plumbing - #336

Merged
cloudsmith-iduffy merged 3 commits into
masterfrom
credential-plumbing
Aug 4, 2026
Merged

refactor(no-ticket): pass resolved credentials through credential-helper plumbing#336
cloudsmith-iduffy merged 3 commits into
masterfrom
credential-plumbing

Conversation

@cloudsmith-iduffy

@cloudsmith-iduffy cloudsmith-iduffy commented Aug 1, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Description

The credential-helper plumbing flattened opts.credential into loose
api_key/auth_type strings, threaded the pair through every signature, then
rebuilt a CredentialResult (with a fabricated source_name) just to hand it to
initialise_api. This passes the resolved CredentialResult itself instead, so
the object carries its own auth scheme (X-Api-Key vs Authorization: Bearer)
end to end. A bearer credential's custom-domain lookup now authenticates as a
bearer token rather than having the scheme re-derived at the bottom of the stack.

Three auth defects surfaced while reviewing the refactor and are fixed here:

  • Stale X-Api-Key leaked into bearer requests. initialise_api() resets
    config.headers on every call but never clears config.api_key, which
    Configuration.set_default() makes sticky. Re-initialising with a bearer
    credential left a previously configured X-Api-Key in place, so subsequent
    requests carried both auth headers. This is reachable via SSO login: the
    initialise_api decorator sets X-Api-Key from credentials.ini, then
    refresh_api_config_after_auth() re-initialises with the bearer token — the
    API could authenticate as the pre-login identity while the CLI reported a
    successful login.

  • Blank-credential guard. CredentialResult is a plain dataclass, so
    if not credential and if org and credential are true for a credential
    carrying an empty api_key; the if not api_key guards they replaced were not
    re-established. Both now check credential.api_key, matching the convention
    the generic helper already uses, so a blank credential can no longer reach an
    unauthenticated custom-domains lookup.

  • install --dry-run was not read-only. Auto-discovery ran before the
    dry_run short-circuit, so a preview issued a live API call and overwrote the
    on-disk domain cache. Discovery is now skipped under dry_run and reported as
    such in the planned actions.

Testing

New coverage: a bearer request asserted clean of a deliberately seeded stale
X-Api-Key; blank-credential and dry_run autodiscovery scenarios asserting the
domains API is never called; and the without-credential domain check given the
httpretty(allow_net_connect=False) and config-path isolation its neighbours
use, so it can no longer pass by falling through to a live 401.

Full suite: 572 passed, 40 skipped.

Type of Change

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update
  • Refactoring
  • Other (please describe)

@cloudsmith-iduffy cloudsmith-iduffy changed the title credential plumbing refactor(no-ticket): pass resolved credentials through credential-helper plumbing Aug 1, 2026
@cloudsmith-iduffy cloudsmith-iduffy mentioned this pull request Aug 1, 2026
Merged
6 tasks
@cloudsmith-iduffy @claude @BartoszBlizniak
refactor(no-ticket): pass resolved credentials through credential-hel…
cb39e1d 
…per plumbing

The credential-helper plumbing flattened opts.credential into loose
api_key/auth_type strings, threaded the pair through every signature,
then rebuilt a CredentialResult (with a fabricated source_name) just to
hand it to initialise_api. The getattr fallback guarding auth_type could
never fire: it is a declared dataclass field with a default.

Pass the CredentialResult itself instead. The object carries its own
auth scheme end to end, so a bearer credential's custom-domain lookup
now goes out as Authorization: Bearer rather than being re-derived at
the bottom of the stack.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
cloudsmith-iduffy and others added 2 commits August 4, 2026 16:01
@cloudsmith-iduffy @claude
fix(no-ticket): clear stale X-Api-Key when switching to a bearer cred…
b86659d 
…ential

initialise_api() resets config.headers on every call but never clears
config.api_key, which Configuration.set_default() makes sticky across
calls. Re-initialising with a bearer credential therefore left a
previously configured X-Api-Key in place, so subsequent requests carried
both auth headers.

This is reachable via the SSO login path: the initialise_api decorator
sets X-Api-Key from credentials.ini, then refresh_api_config_after_auth()
re-initialises with the bearer token. The API could then authenticate as
the pre-login identity while the CLI reported a successful login.

The bearer test previously called unset_api_key() to work around this; it
now seeds a stale key and asserts it is cleared.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cloudsmith-iduffy @claude
fix(no-ticket): guard custom-domain discovery on a usable key and dry…
96a03d0 
…-run

Two defects in the Docker credential-helper install path:

CredentialResult is a plain dataclass, so `if not credential` and
`if org and credential` are true for a credential carrying an empty
api_key — the guards they replaced (`if not api_key`) were not
re-established. Both now check credential.api_key, so a blank credential
no longer reaches an unauthenticated custom-domains lookup.

Auto-discovery also ran before the dry_run short-circuit, so
`install docker --dry-run` issued a live API call and overwrote the
on-disk domain cache despite promising to make no changes. Discovery is
now skipped under dry_run and reported as such in the planned actions.

Also hoists the function-local is_cloudsmith_domain imports to module
level and gives the without-credential test the httpretty and config-path
isolation its neighbours use, so it can no longer pass by falling through
to a live 401.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cloudsmith-iduffy
cloudsmith-iduffy marked this pull request as ready for review August 4, 2026 15:17
@cloudsmith-iduffy
cloudsmith-iduffy requested a review from a team as a code owner August 4, 2026 15:17
Copilot AI lite review requested due to automatic review settings August 4, 2026 15:17
Copilot AI reviewed Aug 4, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors the credential-helper call chain to pass a resolved CredentialResult end-to-end (instead of flattening to api_key/auth_type), ensuring the authentication scheme is preserved throughout custom-domain lookups and API initialization.

Changes:

  • Thread CredentialResult through credential-helper plumbing (install, is_cloudsmith_domain, custom-domain discovery helpers) to avoid re-deriving auth scheme at lower layers.
  • Fix bearer re-initialization behavior so a previously configured X-Api-Key does not leak into bearer-authenticated requests.
  • Make install --dry-run fully read-only by skipping custom-domain auto-discovery (and avoiding cache mutation), with test coverage for the new behaviors.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.

Show a summary per file
File Description
cloudsmith_cli/credential_helpers/docker/runtime.py Passes the resolved credential object into domain checking for Docker helper runtime.
cloudsmith_cli/credential_helpers/docker/installer.py Refactors installer autodiscovery to use CredentialResult and makes dry-run skip discovery/cache writes.
cloudsmith_cli/credential_helpers/custom_domains.py Updates custom-domain lookup APIs to accept CredentialResult directly and initialize auth accordingly.
cloudsmith_cli/credential_helpers/common.py Updates domain checking to require a usable credential (credential.api_key) for custom-domain API lookups.
cloudsmith_cli/core/api/init.py Clears sticky X-Api-Key when switching to bearer auth to prevent mixed-auth headers.
cloudsmith_cli/cli/tests/commands/test_credential_helper.py Adds/updates tests for credential plumbing, bearer header behavior, and blank-credential guards.
cloudsmith_cli/cli/tests/commands/test_credential_helper_install.py Extends installer tests for missing/blank credentials and dry-run autodiscovery behavior; asserts credential passthrough.
cloudsmith_cli/cli/commands/credential_helper/manage.py Passes the resolved credential object to installers instead of flattening fields.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cloudsmith-iduffy
cloudsmith-iduffy merged commit 665d848 into master Aug 4, 2026
27 checks passed
@cloudsmith-iduffy
cloudsmith-iduffy deleted the credential-plumbing branch August 4, 2026 15:37
cloudsmith-iduffy added a commit that referenced this pull request Aug 4, 2026
@cloudsmith-iduffy @claude
merge: bring in master's credential plumbing refactor (#336)
476c9d0 
Both sides reworked the same credential-helper call sites. Master's
versions are supersets in every conflict — the api_key guard on
is_cloudsmith_domain, the dry-run skip for auto-discovery, and the
stale-key test that seeds a key rather than clearing it — so they win;
the branch's strict-mode custom-domain tests are kept alongside.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@cloudsmith-iduffy cloudsmith-iduffy mentioned this pull request Aug 9, 2026
Draft
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@BartoszBlizniak BartoszBlizniak BartoszBlizniak approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cloudsmith-iduffy @BartoszBlizniak